-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
328, 390, 373: BIPs for MuSig2 derivation, descriptors, and PSBT fields #1540
Conversation
It would be helpful to document the reasoning in the commit msg or here in the PR to have a record of design decisions. |
61fe36a
to
6b482ea
Compare
I've added a rationale section which addresses this. |
bip-musig2-derivation.mediawiki
Outdated
However, it is much simpler to be able to derive from a single extended public key instead of having | ||
to derive from many extended public keys and aggregate them. As MuSig2 produces a normal looking | ||
public key, the aggregate public can be used in this way. This reduces the storage and computation | ||
requirements for generating new aggregate pubkeys. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An additional benefit is that such wallets can be watched by a musig2-unaware wallet, or a tapleaf co-signer who does not need to know about the individual keys, by casting the tr(musig())
descriptor to tr()
. Similarly it's compatible with the privacy-preserving rawtr
descriptor (needs a BIP).
I've made a minor change with the descriptors regarding sorting. Keys in a |
What is the rationale for this change? It seems to me that this complicates parsing/processing the descriptor unnecessarily. The only practical advantage I know of |
I had a discussion involving @jonasnick and @sipa about this. Since BIP 327 suggests to do sorting, we decided it made sense to do in descriptors as well. |
BIP 327 discusses the possibility of sorting and describes a canonical sorting, but I didn't read it as 'suggesting' one way or another. In general, I'd prefer maximum simplicity and less chances of malleability in descriptors, and this imho goes against both (albeit slightly, not a huge deal). Note that this removes functionality, as one of |
The functionality that I would suggest to consider for removal is derivation before aggregation: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-November/022132.html Unless there are use cases for it that I'm not aware of? |
script were derived. | ||
| <tt><33 byte compressed pubkey>*</tt> | ||
| A list of the compressed public keys of the participants in the MuSig2 aggregate key in the order | ||
required for aggregation. If sorting was done, then the keys must be in the sorted order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the specification for the descriptor says that keys must be sorted with KeySort
. If unsorted keys are allowed here, then its possible to have a psbt for a musig aggregate key that's not expressible in a descriptor. Maybe that's ok, just flagging it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is allowed. PSBTs is intentionally less restrictive than descriptors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's wrong with taking the ordering from the descriptor directly? That seems simpler and more flexible (Not that I can imagine a specific reason why a user would want a particular order).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is allowed. PSBTs is intentionally less restrictive than descriptors.
makes sense, you want PSBTs to be a super-set of what you can express in a descriptor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's wrong with taking the ordering from the descriptor directly? That seems simpler and more flexible (Not that I can imagine a specific reason why a user would want a particular order).
There's no descriptor in psbts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right I was commenting on KeySort-ing the participant keys in the descriptor. Seemed on-topic for this thread but I'll put the comment where it belongs.
bip-musig2-descriptors.mediawiki
Outdated
expression as a key expression. The aggregate public key is produced by using the <tt>KeyAgg</tt> | ||
algorithm on all KEYs specified in the expression after performing all specified derivation. As with | ||
script expressions, KEY can contain child derivation specified by <tt>/*</tt>. A new aggregate | ||
public key will be computed for each child index. Keys must be sorted with the <tt>KeySort</tt> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why KeySort when we already have a explicit ordering from the descriptor? It seems like an unnecessary complication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in #1540 (comment), there was a discussion about this and that was the conclusion. But I'm also having a hard time remembering the exact reason, maybe @jonasnick and/or @sipa can help me out here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talking about this again with @sipa, the conclusion is that philosophically, MuSig2 operates over a set of keys, so the order should not matter. For serialization as an implementation detail, it does, so having it sorted is a logical order for an ostensibly unordered set.
Another argument for sorting is to make the recovery process easier,. As we know from multisig usage that exists currently, users are often surprised to learn that they had to know more than just their key and their cosigners. By sorting, we can eliminate this as a concern. Ostensibly, users should be backing up their descriptors, but in practice, especially with the prevalance of hardware signers which espouse that only the seed needs to be backed up, I don't think that's something that happens that often.
The formatting seems to be in order on these documents. What is the status of the discussion on these proposals? |
Given that there are still a few active questions and some things may still end up being changed, I prefer to wait a bit longer before committing to the current specs with test vectors. There are a few for the descriptors BIP.
This was considered originally, however there is a dependency on the derivation BIP from both the descriptor and psbt BIPs, so I'm not sure it makes sense to have them be separate. |
BIP-0327 allows participant pubkeys to be duplicates. Since the psbt fields are using the pubkeys (and not the signer's position in the MuSig) in order to identify the nonce/partial_sig contributions, and given that duplicate pubkeys are AFAIK of no practical value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s call these:
• BIP 328: Derivation Scheme for MuSig2 Aggregate Keys
• BIP 373: MuSig2 PSBT Fields
• BIP 390: musig() Descriptor Key Expression
0b28f51
to
c6461ea
Compare
Updated with assigned numbers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a closer look at the PSBT fields while implementing them for the Golang psbt
library in btcd
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 1542fda
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a full read-through of all three BIPs. I noted a few open questions and minor formatting suggestions. I noticed that some of the proposals do not go into detail on design decisions, alternate designs that were considered or related work. While the proposals seem straightforward, I thought I mentioned it in case there are things to be added to the rationale section that were forgotten.
It’s not clear to me whether the presence of test vectors would be necessary for the specification to be considered complete, and I noticed that this PR and the mailing list post got little discussion so far. It seems obvious to me that all of these proposals are relevant, so I was wondering whether you would like us to wait for more review or the addition of test vectors before considering merging, or whether you consider this PR ready to be merged—regarding the formal criteria for BIPs, these seem ready to go.
Added a sentence for that.
That's not been an issue before. Previous draft proposals have been merged without test vectors completed yet.
I consider these ready for merging. |
This PR contains 3 proposed BIPs, the main contents of which were sent to the bitcoin-dev mailing list in October. There have been a few changes, notably the addition of a new BIP for the derivation methodology which was split from the descriptors BIP.
I've also changed the PSBT BIP to use 33 byte plain aggregate pubkeys rather than xonly.